fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content by abi-jey · Pull Request #1416 · microsoft/apm

abi-jey · 2026-05-20T09:25:23Z

Summary

When a package has both apm.yml and .claude-plugin/plugin.json with manifest paths pointing into .apm/, apm install silently drops all agents and skills — reporting (files unchanged).

Root Cause

detect_package_type classifies packages with .claude-plugin/plugin.json as MARKETPLACE_PLUGIN (cascade priority — checked before APM_PACKAGE), even when apm.yml is present. This triggers _validate_marketplace_plugin → normalize_plugin_directory → _map_plugin_artifacts.

_map_plugin_artifacts is designed for pure plugins where agents/ and skills/ sit at the package root and need to be copied INTO .apm/. It does a shutil.rmtree on the target (.apm/agents/, .apm/skills/) before copying from the source. When the manifest points to paths already inside .apm/, the source and target overlap — the rmtree destroys the source before the copy can read it.

Sequence:

Download copies package to apm_modules/ — .apm/agents/ and .apm/skills/ are present
validate_apm_package → _map_plugin_artifacts runs
shutil.rmtree(.apm/agents/) — deletes the source
shutil.copy2(source_file, ...) — source no longer exists, nothing is copied
Integrate phase finds empty .apm/ → (files unchanged)

Fix

Added _all_inside_apm() helper that detects when all resolved source paths already reside under .apm/. When true, the destructive rmtree+copy cycle is skipped — the artifacts are already correctly positioned. Applied to all four component types: agents, skills, commands, hooks.

Tests

5 new regression tests in TestMapPluginArtifactsPrePositioned:

test_agents_inside_apm_are_preserved — manifest agents pointing into .apm/ survive
test_skills_inside_apm_are_preserved — manifest skills pointing into .apm/ survive
test_commands_inside_apm_are_preserved — manifest commands pointing into .apm/ survive
test_hooks_inside_apm_are_preserved — manifest hooks pointing into .apm/ survive
test_external_agents_still_copied — normal root-level→.apm/ copy still works (non-regression)

All 4 regression tests fail without the fix, pass with it. All 73 plugin parser tests pass.

Copilot

Pull request overview

Fixes a destructive edge case in plugin artifact normalization during apm install when plugin.json component paths point into an already-populated .apm/ tree, preventing accidental deletion of pre-positioned agents/skills/prompts/hooks.

Changes:

Added a containment guard in _map_plugin_artifacts() to skip the rmtree + copy cycle when sources appear to already live under .apm/.
Added regression tests ensuring .apm/ content is preserved for agents, skills, commands (prompts), and hooks.
Added a non-regression test confirming root-level agents/ are still copied into .apm/agents/.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/apm_cli/deps/plugin_parser.py	Adds `_all_inside_apm()` and uses it to conditionally skip destructive remapping for agents/skills/commands/hooks.
tests/unit/test_plugin_parser.py	Adds regression coverage for pre-positioned `.apm/` artifact preservation and one non-regression copy test.

…oft#1415) The `rmtree(target); copytree(source, target)` cycle wiped the source when a hybrid APM + Claude-plugin manifest pointed paths INTO `.apm/`, causing `apm install` to silently report `(files unchanged)`. The rmtree was redundant -- the downloader already clears install_path before extraction. Replaced with `dirs_exist_ok=True` plus an `_is_same_path` guard for `copy2` self-copies. Tests: 6 regression cases (pre-positioned + mixed-source layouts).

danielmeppiel · 2026-05-21T22:06:26Z

APM Review Panel: `ship_with_followups`

Community-contributed fix closes a silent data-loss regression in hybrid-package install; removes unconditional rmtree that destroyed pre-positioned .apm/ content, with a credible defense-in-depth symlink follow-up to track.

cc @abi-jey @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

This PR closes a severity-high data-loss bug (#1415) where apm install silently destroyed user-authored .apm/agents and .apm/skills content for packages shipping both apm.yml and .claude-plugin/plugin.json with manifest paths into .apm/. The fix is minimal, targeted, and backed by 7 passing unit-tier regression tests. Six of seven active panelists converge on ship: the correctness gain is unambiguous and the install-adds-never-silently-mutates promise is restored.

The sole blocking-severity finding comes from supply-chain-security-expert: removing rmtree of the destination directory means a malicious package that ships .apm/skills/<name> as a symlink can redirect copytree writes outside the plugin boundary. This is a real exploit surface in theory. In practice, exploitation requires a crafted package whose extracted tree contains a dst-side symlink that survives extraction AND passes existing src-side symlink guards. The broader install pipeline likely sanitizes symlinks at the tar/git-extraction layer, but confirming that requires tracing code this PR does not touch. The panel weighs it as a credible defense-in-depth gap -- high enough signal to track as a blocking-tier follow-up issue opened immediately after merge, but NOT high enough to block a targeted data-loss fix from shipping.

The stale-file-on-upgrade trade-off (architect, devx, security all converge) is the correct trade-off for this PR: data preservation beats stale-file cleanup. Document upgrade semantics in a follow-up and consider a manifest-diff-driven prune pass in a subsequent PR.

Dissent. supply-chain-security-expert raised a BLOCKING finding (dst-symlink write-anywhere primitive) that no other panelist surfaced; python-architect and devx-ux-expert noted stale-file persistence but missed the symlink vector. The panel sides with treating the symlink concern as a real defense-in-depth gap worthy of an immediate follow-up issue (blocking-tier emphasis in rendering) rather than a merge-gating defect.

Aligned with: secure-by-default (restores install-never-destroys-user-content; surfaces a defense-in-depth symlink gap for follow-up), pragmatic-as-npm (hybrid-package installs now behave predictably -- pre-positioned content survives, matching user mental model of additive install), oss-community-driven (community contributor @abi-jey identified, diagnosed, and fixed a data-loss regression with regression tests; exemplary contribution pattern).

Growth signal. High-signal contributor win: a community member identified a silent data-loss bug in the install pipeline, provided a minimal fix with 7 regression tests, and followed responsible-disclosure norms by filing #1415 first. Amplify in the next release note with explicit credit to @abi-jey and frame the fix under "silent data loss" language in CHANGELOG to reassure adopters that the install path is trustworthy.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	5	1	Fix is structurally sound; flag staleness invariant, extract `_is_same_path` module-level, and dedupe the 5x branch shape.
CLI Logging Expert	0	2	1	Silent-by-design is correct at default verbosity; add a debug breadcrumb and surface the swallowed OSError.
DevX UX Expert	0	3	1	Restores the "install adds, never silently mutates" promise; ship. Flag stale-file-on-upgrade gap and recovery messaging.
Supply Chain Security	1	2	1	Removing rmtree turns pre-existing dst symlinks into a write-anywhere primitive via `copytree` merge. Scope the skip narrowly.
OSS Growth Hacker	0	2	1	High-signal community contribution: data-loss fix with regression tests. Ship, amplify in CHANGELOG/release notes.
Doc Writer	0	1	0	Missing CHANGELOG Unreleased > Fixed entry for the hybrid-package data-loss fix.
Test Coverage Expert	0	2	1	Unit-tier coverage of the helper is thorough (7 pass); add one integration-with-fixtures test on the install pipeline.
Performance Expert	0	0	1	One-shot install path; no perf-meaningful surface touched.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

[Supply Chain Security] (blocking-severity) Harden dst-symlink handling: validate that copytree destinations inside .apm/ are not symlinks before writing, or scope the rmtree-skip to only apply when src resides inside target. -- Removing rmtree without dst-side symlink validation opens a write-anywhere primitive for crafted packages. Defense-in-depth gap on a secure-by-default surface.
[Doc Writer] Add CHANGELOG Unreleased > Fixed entry naming symptom (silent data loss for hybrid apm.yml + plugin.json packages), mechanism, and refs (fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content #1416 closes [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415). -- Three panelists (doc-writer, oss-growth-hacker, devx-ux-expert) converge: the data-loss fix must be visible in release artifacts to rebuild trust with affected users.
[Test Coverage Expert] Add integration-with-fixtures test exercising the full install pipeline on a dual-manifest fixture package with pre-positioned .apm/ content. -- Current 7 tests are unit-tier only; the actual user-visible failure from [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415 traverses validate_apm_package -> _map_plugin_artifacts -> filesystem. Missing integration coverage on a critical-promise surface.
[Python Architect] Document or enforce stale-file-on-upgrade semantics: either add a manifest-diff prune pass or document that orphan files persist across plugin version upgrades. -- Removing rmtree trades data-loss for stale-file persistence. Acceptable for this PR but needs explicit semantics before the upgrade path is considered stable.
[CLI Logging Expert] Add debug-tier breadcrumbs when _is_same_path triggers copy-skip and when OSError is swallowed, so the next [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415-shaped regression is diagnosable without a debugger. -- Two panelists (cli-logging, supply-chain) converge on OSError-swallow risk; debug logging makes fail-open observable.

Architecture

classDiagram
    class normalize_plugin_directory {
        <<EntryPoint>>
        +Path plugin_path
        +parse manifest
        +delegate to synthesize_apm_yml_from_plugin
    }
    class synthesize_apm_yml_from_plugin {
        <<Coordinator>>
        +ensure .apm dir
        +call _map_plugin_artifacts
        +emit apm.yml
    }
    class _map_plugin_artifacts {
        <<TransformPipeline>>
        +copy agents -> .apm/agents
        +copy skills -> .apm/skills
        +copy commands -> .apm/prompts (normalize .md)
        +copy hooks -> .apm/hooks (dir|file|inline)
        +copy passthrough files
    }
    class _resolve_sources {
        <<Closure>>
        +manifest-driven or default-dir
        +PathTraversal guard via _is_within_plugin
    }
    class _is_same_path {
        <<Closure-PureFn>>
        +resolve src == resolve dst
        +OSError -> False
        SHOULD BE MODULE-LEVEL
    }
    class _copy_command_file {
        <<Closure>>
        +normalize .md to .prompt.md
        +mkdir parents
    }
    class ignore_non_content {
        <<Predicate from security.gate>>
        +skip symlinks
        +skip .apm-pin
    }
    normalize_plugin_directory --> synthesize_apm_yml_from_plugin
    synthesize_apm_yml_from_plugin --> _map_plugin_artifacts
    _map_plugin_artifacts ..> _resolve_sources : nested
    _map_plugin_artifacts ..> _is_same_path : nested
    _map_plugin_artifacts ..> _copy_command_file : nested
    _map_plugin_artifacts --> ignore_non_content : injected predicate

flowchart TD
    A[plugin_path - freshly extracted tarball or in-repo checkout] --> B[normalize_plugin_directory]
    B --> C[parse_plugin_manifest - .claude-plugin/plugin.json]
    C --> D[synthesize_apm_yml_from_plugin]
    D --> E[mkdir .apm/]
    D --> F[_map_plugin_artifacts]
    F --> G{manifest entry source vs .apm target}
    G -->|src outside .apm| H[copytree dirs_exist_ok=True]
    G -->|src INSIDE .apm == dst| I[_is_same_path True -> skip]
    G -->|src INSIDE .apm but different subpath| J[copytree - may union-merge stale]
    H --> K[.apm/agents .apm/skills .apm/prompts .apm/hooks]
    I --> K
    J --> K
    K --> L[_generate_apm_yml -> apm.yml]
    L --> M[apm package ready]
    style I fill:#cfc,stroke:#080
    style J fill:#ffd,stroke:#880
    J -.->|staleness risk| N[Files dropped between plugin versions persist in .apm/. Old rmtree mirrored; new union-merge does not.]
    style N fill:#fee,stroke:#800

Recommendation

Merge this PR to close the active data-loss regression for all users of hybrid-manifest packages. Open a follow-up issue immediately for dst-symlink hardening (the supply-chain blocking finding) and assign it to the next milestone. The maintainer may optionally ask @abi-jey to add the CHANGELOG entry in this PR before merge; all other follow-ups land in subsequent PRs.

Full per-persona findings

Python Architect

[recommended] Staleness invariant silently dropped: removing rmtree turns _map_plugin_artifacts from 'mirror sources into .apm/' into 'merge sources over .apm/', with no replacement for the cleanup it provided. at src/apm_cli/deps/plugin_parser.py:452
The old pattern if target.exists(): rmtree(target) was load-bearing in two ways: (1) DATA-LOSS bug, correctly fixed by this PR; (2) STALENESS GUARANTEE on re-install. PR's 7 new tests all assert single-call preservation; none re-invoke _map_plugin_artifacts a second time to assert stale entries from a prior call are absent. Whether this regresses depends on caller invariant.
Suggested: Either document in the docstring that callers MUST pass a freshly extracted plugin_path, OR add a pre-copy prune pass: for each target dir, remove any descendant whose corresponding source-relative path is absent from the union of sources.
[recommended] _is_same_path is a stateless pure helper buried as a nested closure inside _map_plugin_artifacts; it captures nothing and is used in 7 branches. at src/apm_cli/deps/plugin_parser.py:448
Nested function scope is appropriate when a helper closes over enclosing-scope state. _is_same_path closes over nothing. Hiding a pure utility inside a 180-line function prevents unit-testing it directly, blocks reuse by sibling functions, and violates the same hygiene that motivated extracting _resolve_sources at module level.
Suggested: Move to module scope as _paths_equal_resolved(src, dst) -> bool (or to apm_cli/utils/paths.py alongside portable_relpath); add focused unit tests covering identical, different, broken-symlink (OSError), and non-existent paths.
[recommended] The five branches now share an identical 'iterate sources, skip if _is_same_path, copytree(dirs_exist_ok=True, ignore=ignore_non_content)' shape -- a pylint R0801 hazard against the canonical linting contract. at src/apm_cli/deps/plugin_parser.py:459
The repo's linting instructions explicitly call out the R0801 gate. Even if R0801 does not trip today, the pattern stereotype is screaming for extraction: _copy_dirs_into(sources, target, *, ignore) and _copy_file_into(source_file, target_path) pair would compress _map_plugin_artifacts by ~30 lines and give each shape one place to evolve.
Suggested: Extract _copy_dirs_into and _copy_file_into helpers; call from agents/skills(default)/hooks/pass-through branches; skills custom-list branch becomes _copy_dirs_into([d], target_skills / d.name, ignore=...) per dir.
[recommended] commands branch: .md -> .prompt.md normalization is not _is_same_path-symmetric, so a pre-positioned manifest entry at .apm/prompts/foo.md (not yet normalized) is duplicated rather than skipped. at src/apm_cli/deps/plugin_parser.py:525
_copy_command_file derives target_path from source_file.name then conditionally renames .md to .prompt.md. _is_same_path is checked AFTER the rename. A plugin author who pre-positioned .apm/prompts/foo.md (legal under spec) will end up with both .apm/prompts/foo.md (original, orphaned) AND .apm/prompts/foo.prompt.md (copy).
Suggested: In _copy_command_file, after computing target_path: if _paths_equal_resolved(source_file, target_path): return. Additionally, if source_file is inside dest_dir and target_path differs only by the .prompt suffix, prefer in-place rename over copy+leave-original.
[recommended] No regression test asserts the SECOND-call behavior of _map_plugin_artifacts (idempotency under re-normalization). at tests/unit/test_plugin_parser.py:967
TestMapPluginArtifactsPrePositioned proves first-call preservation but not idempotency. A natural test: 'call _map_plugin_artifacts twice with the same manifest pointing inside .apm/; assert no SameFileError and no duplicate/corrupt content', plus 'call once with manifest_v1 including extra.agent.md, again with manifest_v2 lacking it; assert extra.agent.md is gone' (would FAIL today, proving the staleness gap).
Suggested: Add test_repeated_invocation_is_idempotent and test_stale_files_from_prior_version_removed (xfail or skip if staleness invariant is intentionally relaxed).
[nit] _is_same_path uses resolve() which does not detect hardlinks (two distinct paths, same inode); shutil.copy2 will then raise SameFileError uncaught. at src/apm_cli/deps/plugin_parser.py:448
resolve() canonicalizes symlinks and casing but returns distinct strings for hard-linked files. os.path.samefile(src, dst) covers both the symlink-followed case AND the hardlink case in one syscall.
Suggested: Replace body with try: return os.path.samefile(src, dst); except (OSError, FileNotFoundError): return False.

CLI Logging Expert

[recommended] Add a _logger.debug() breadcrumb when _is_same_path triggers a copy-skip, so pre-positioned .apm/ layouts leave a diagnosable trace. at src/apm_cli/deps/plugin_parser.py:451
The bug behind [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415 was catastrophic precisely because the destructive path was silent end-to-end. The fix preserves the silence: _is_same_path short-circuits with continue / return and emits nothing. At debug verbosity a one-liner makes the next regression of this shape cheap to diagnose. plugin_parser.py already wires _logger = logging.getLogger(__name__) and uses it for similar breadcrumbs.
Suggested: Inside _is_same_path, after True comparison, emit _logger.debug("Skipping copy: source already at destination: %s", src). Debug-tier only; no _rich_* surface, no STATUS_SYMBOLS.
[recommended] _is_same_path swallows OSError silently and returns False, which falls through to a real shutil.copy* -- log it at debug so a resolve() failure does not hide as a phantom copy. at src/apm_cli/deps/plugin_parser.py:453
If .resolve() fails for a real reason (symlink loop, permission denied, Windows reparse-point oddity), the user sees a copy attempt that may itself raise SameFileError or partially clobber state -- with no log to explain why the guard did not trigger. Same defensive-logging principle: silent failure of a data-loss guard is exactly the failure mode [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415 taught us not to repeat.
Suggested: Change the except arm to except OSError as exc: _logger.debug("_is_same_path resolve() failed for %s vs %s: %s", src, dst, exc); return False.
[nit] Consider whether the integrate-phase (files unchanged) wording is still accurate when content was already pre-positioned in .apm/ -- it now reports 'unchanged' for two semantically different cases.
Outside the diff. With this fix, integrate reaches (files unchanged) both when (a) re-install produced byte-identical copies and (b) the package was a pre-positioned APM layout where nothing was ever copied. Case (b) is the new branch this PR introduces. Not blocking.

DevX UX Expert

[recommended] Recovery story for users hit by the data-loss bug is not addressed; CHANGELOG/release notes must call it out. at CHANGELOG.md
Users who already ran the broken version have no way to discover the loss other than noticing their content is gone -- and apm install printed success. The release that ships this fix MUST tell affected users (a) the symptom, (b) the one concrete recovery action (git restore .apm/agents .apm/skills), and (c) which versions are affected.
Suggested: Add an Unreleased > Fixed entry naming symptom, affected versions, and recovery command. Mirror in the GitHub release body.
[recommended] Loss of rmtree-then-copytree means stale files now persist across re-installs; idempotency improved but 'upgrade removes' semantics regress silently. at src/apm_cli/deps/plugin_parser.py:462
The pre-fix code gave clean-slate semantics: a package version bump that REMOVED an agent file would also remove it from the materialized .apm/ tree on re-install. After this PR, dirs_exist_ok=True is additive only. This is the right trade-off versus the data-loss bug, but apm install after apm update to a newer package version may leave ghost files. Worth a follow-up to either clean apm_modules/<pkg>/.apm/ before re-extracting OR document as a known sharp-edge.
[recommended] Test matrix is missing the .mcp.json / .lsp.json / settings.json pass-through pre-positioned case. at tests/unit/test_plugin_parser.py:1050
The fix correctly added a _is_same_path guard for the three pass-through files, but the new test class covers agents/skills/commands/hooks/hybrid -- not pass-throughs. A future refactor that drops the guard there would silently regress.
Suggested: Add test_passthrough_files_inside_apm_are_preserved that pre-positions .apm/.mcp.json, invokes _map_plugin_artifacts, and asserts file content is unchanged.
[nit] Silent skip on same-path leaves no diagnostic for future triage; consider a debug log. at src/apm_cli/deps/plugin_parser.py:451
A single _rich_info or debug-level log under verbose mode would aid the next triage cycle at zero UX cost on the default path. Defer to CLI Logging UX expert on the exact channel.

Supply Chain Security Expert

[blocking] Removing rmtree of target_* turns pre-positioned dst symlinks into a write-anywhere primitive via copytree(dirs_exist_ok=True). at src/apm_cli/deps/plugin_parser.py:462
Pre-PR, shutil.rmtree(target_skills) unlinked any symlinks sitting in the destination tree before copy began (rmtree removes symlinks by default). Post-PR the dst tree is preserved verbatim and merged with copytree(..., dirs_exist_ok=True, ignore=ignore_non_content). A malicious package can ship .apm/skills/<name> as a symlink to /etc or $HOME/.ssh, and a real file under skills/<name>/payload. _resolve_sources only validates SOURCE paths; copytree writes through the dst symlink. _is_same_path does not catch this because src and dst resolve to different paths.
Suggested: Scope the rmtree skip to the legitimate case: should_clean = not any(_is_within(src.resolve(), target.resolve()) for src in sources); if should_clean and target.exists(): shutil.rmtree(target). The pre-positioned APM-package case sets should_clean=False; every other case (including malicious dst symlinks) gets the destructive cleanup. Alternative: walk target_* before each copy and refuse/strip any subpath whose lstat is a symlink.
Proof (missing at): tests/unit/test_plugin_parser.py::TestMapPluginArtifactsPrePositioned::test_dst_symlink_in_target_does_not_redirect_copy -- proves: A malicious package cannot redirect file writes outside the plugin root by pre-positioning a symlink at .apm/skills/<name> pointing at an external directory. [secure-by-default, governed-by-policy]
assert not (external_target / 'payload').exists(), 'copytree followed dst symlink and wrote outside plugin root'
[recommended] Stale-file persistence: dropping rmtree leaves orphaned content from prior installs of the same package. at src/apm_cli/deps/plugin_parser.py
Threat model: a typosquatting/compromise variant where v1 of package A ships malicious-helper.agent.md and v2 (a remediation release) removes it from the manifest; a user upgrading from v1 to v2 still has the malicious file in .apm/ and downstream auto-integrators continue to deploy it.
Suggested: Either keep the narrow-scope rmtree (see blocking finding), or document in plugin_parser docstring that callers MUST guarantee a clean staging dir before invocation; add CHANGELOG note about upgrade semantics.
Proof (missing at): tests/unit/test_plugin_parser.py::TestMapPluginArtifactsPrePositioned::test_v2_install_removes_v1_orphan_agent -- proves: Re-running _map_plugin_artifacts with a manifest no longer listing an agent removes the v1 file from .apm/agents/. [secure-by-default, governed-by-policy]
[recommended] _is_same_path swallows OSError silently; a denied-permission lookup is treated as not-same and the copy proceeds. at src/apm_cli/deps/plugin_parser.py:453
Swallowing OSError silently hides the security-relevant case where dst is a broken/dangling symlink (a plausible signal of tampering or partial prior extract). Log at debug or warning level (without the resolved path string -- manifest values are tainted).
Suggested: Consider returning True (fail-safe -- skip the copy) for the inside-target case rather than False (fail-open) when resolve() fails on dst.
[nit] Test count bump 7 -> 5 in test_symlink_containment.py is legitimate; comment is now stale. at tests/unit/test_symlink_containment.py:286
The new code consolidates the prior 'first + rest' copytree patterns into single loops; the four remaining copytree call sites plus the ignore_non_content import correctly yield 5 references. The reduction is not a guardrail regression -- ignore=ignore_non_content is still applied to every copytree. But the inline comment still references 7 calls and >=8 references.
Suggested: Update the comment to reflect post-fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content #1416 shape: '4 copytree calls in total; ALL 4 use ignore_non_content; import + 4 call sites yields >= 5 references'.

OSS Growth Hacker

[recommended] Call out this fix prominently in CHANGELOG under Fixed, with [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415/fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content #1416 and 'silent data loss' language. at CHANGELOG.md
Data-loss bugs in apm install are the worst-class adoption killer. Shipping the fix is necessary but not sufficient -- the CHANGELOG line is what reaches users who got burned (or were about to) and converts a near-miss into a trust signal. Use explicit user-words.
Suggested: Under [Unreleased] -> Fixed: 'apm install: prevent silent data loss when a package declares both apm.yml and .claude-plugin/plugin.json with manifest paths inside .apm/. The plugin normalizer previously rmtree'd the source before copy, dropping all agents/skills with a misleading (files unchanged) summary. Fixes [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415. Thanks @abi-jey.'
[recommended] Promote this fix in the next release note narrative -- not just the bullet list -- and credit the contributor.
Two adoption-funnel wins compound: (1) a data-loss class bug closed protects evaluators trying APM, and (2) a community contributor drove root-cause analysis, sequence walkthrough, and 5+ regression tests including the hybrid case suggested in review. That is the exact story APM wants to tell about its contributor experience.
Suggested: When the next release note is drafted, dedicate a one-paragraph 'Notable fixes' section to fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content #1416 with contributor attribution.
[nit] Consider an explicit thank-you in the PR review comment and a 'good first PR' style acknowledgement.
@abi-jey delivered an exemplar contribution: linked issue, root-cause walkthrough, targeted helper, regression tests for all four artifact types plus non-regression case. A visible 'this is how we love to receive bug fixes' note in the merge comment costs nothing and seeds repeat contribution behavior.

Auth Expert -- inactive

Touched files (src/apm_cli/deps/plugin_parser.py, tests/unit/test_plugin_parser.py, tests/unit/test_symlink_containment.py) are plugin manifest parsing and symlink containment tests; no auth fast-path file or credential/host/token surface is modified.

Doc Writer

[recommended] Missing CHANGELOG Unreleased > Fixed entry for hybrid-package data-loss fix. at CHANGELOG.md
CHANGELOG.md currently has an ## [Unreleased] block with a ### Fixed subsection containing multi-sentence entries for recent user-visible bug fixes (e.g. apm update ADO Windows fix [BUG] apm update ADO bearer auth fails on Windows: subprocess.run can't resolve az.cmd #1430, root .apm hooks duplication fix(hooks): stabilize root .apm hook source-ids across renames/worktrees (supersedes #1330, closes #1329) #1392). PR fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content #1416 fixes a silent data-loss bug in apm install for packages that ship BOTH apm.yml AND .claude-plugin/plugin.json with manifest paths into .apm/ (closes [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415), but no entry is added under ### Fixed. .github/instructions/changelog.instructions.md governs that file and treats notable bug fixes as Fixed entries.
Suggested: Add an Unreleased > Fixed entry naming the symptom (silent (files unchanged) install for packages with both apm.yml and .claude-plugin/plugin.json whose manifest points into .apm/), the root cause one-liner (_map_plugin_artifacts rmtree'd the source before copy because source and target overlapped inside .apm/), and refs (#1416, closes #1415).

Test Coverage Expert

[recommended] Add an integration-with-fixtures test that calls validate_apm_package on a dual-manifest package with pre-positioned .apm/ content (the actual [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415 scenario). at tests/integration/test_marketplace_plugin_integration.py
The install pipeline is a critical-promise surface; the tier floor is integration-with-fixtures, not unit. The 7 new tests call _map_plugin_artifacts directly with synthetic tmp_path inputs -- they pin the helper but do not exercise the upstream chain validate_apm_package -> normalize_plugin_directory -> _map_plugin_artifacts. A future refactor that wires the helper incorrectly would silently regress the user promise while the 7 unit tests stay green. No existing fixture matches the failing profile.
Suggested: Add a test creating a fixture with apm.yml, .claude-plugin/plugin.json declaring agents/skills/commands/hooks pointing into .apm/, and pre-positioned content; call validate_apm_package(pkg_dir); assert pre-positioned files still exist and contents are unchanged.
Proof (missing at): tests/integration/test_marketplace_plugin_integration.py -- proves: apm install on a real dual-manifest package with content pre-positioned in .apm/ does not destroy that content -- the actual user scenario reported in [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415. [secure-by-default, devx, portability-by-manifest]
result = validate_apm_package(pkg_dir); assert (pkg_dir / '.apm' / 'agents' / 'my-agent.agent.md').exists(); assert (pkg_dir / '.apm' / 'skills' / 'my-skill' / 'SKILL.md').exists()
[recommended] Add a regression test asserting that a package with BOTH apm.yml and .claude-plugin/plugin.json is classified as APM_PACKAGE (not MARKETPLACE_PLUGIN). at tests/unit/test_apm_package.py
Issue [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415's root cause analysis cites classifier ordering as a contributing factor. The PR fixes the symptom at the copy boundary, which is the right defense-in-depth choice, but leaves no test pinning the classifier verdict for the dual-manifest case. If a future change reverts classifier ordering, dual-manifest packages will be reprocessed through marketplace normalization with potential side effects (apm.yml synthesis overwriting, metadata loss).
Suggested: Add a classifier test: build a tmp_path package with apm.yml AND .claude-plugin/plugin.json, call detect_package_type(pkg_dir), assert PackageType.APM_PACKAGE.
Proof (missing at): tests/unit/test_apm_package.py -- proves: Dual-manifest packages route through APM_PACKAGE normalization, preventing silent marketplace-path reprocessing that contributed to [BUG]: cluade Plugin.json + .apm directory in the same package causes apm install to delete the .apm directory #1415. [portability-by-manifest, secure-by-default]
assert detect_package_type(pkg_dir) == PackageType.APM_PACKAGE # dual-manifest: apm.yml wins over .claude-plugin/plugin.json
[nit] TestMapPluginArtifactsPrePositioned passes 7/7 on the PR commit; the suite is well-scoped per component plus external-still-copied and hybrid mixed-source. at tests/unit/test_plugin_parser.py
Verified by running the new class on the PR branch. The hybrid mixed-source test is the most valuable addition because it pins the per-source overlap behavior. The ignore_non_content reference-count adjustment in test_symlink_containment.py (7 -> 5) is a real, semantically-correct delta.
Proof (passed): tests/unit/test_plugin_parser.py::TestMapPluginArtifactsPrePositioned -- proves: _map_plugin_artifacts no longer destroys pre-positioned .apm/ content when the manifest points into .apm/, across all five component branches and in mixed-source manifests. [secure-by-default, devx]
Run: uv run --extra dev pytest tests/unit/test_plugin_parser.py::TestMapPluginArtifactsPrePositioned -v -> 7 passed in 0.85s

Performance Expert

[nit] Cache resolved target paths to avoid repeated resolve() per source. at src/apm_cli/deps/plugin_parser.py
Minor: dst.resolve() is recomputed for every source in agent_dirs / skill_dirs / hook_sources / command files. Targets are loop-invariant, so resolving them once outside the loop would shave a handful of stat() calls per install. Cosmetic, not a measurable hotspot -- plugin install is one-shot and copy I/O dominates. Leave as-is unless a follow-up touches this function.

_{This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.}

Defense-in-depth follow-up to apm-review-panel finding on microsoft#1416: the supply-chain reviewer flagged that removing the unconditional rmtree leaves pre-existing dst symlinks inside target_agents / target_skills / target_prompts / target_hooks unvalidated, so a malicious package shipping .apm/<component>/<name> as a symlink to an external path (e.g. /etc, $HOME/.ssh) can redirect shutil.copytree(..., dirs_exist_ok=True) writes outside the plugin root. Add _assert_no_symlink_descendants(target) which walks target with os.walk(followlinks=False) and refuses to copy when target itself or any descendant is a symlink. The new PluginIntegrityError is raised before each copytree / copy2 site that writes into apm_dir (agents, skills, prompts, hooks, .mcp.json / .lsp.json / settings.json). Raising is preferred over silent-skip because the threat model is data-loss-adjacent: a redirected write is more dangerous than a failed install. Regression-trapped by test_dst_symlink_in_target_does_not_redirect_copy which stages target_agents/linked -> external_sentinel/, calls _map_plugin_artifacts on a package that ships agents/linked/evil.md, and asserts PluginIntegrityError fires AND the sentinel is unmodified. Mutation-break gate confirmed: removing the guard at the agents copy site flips the test from PASS to 'DID NOT RAISE'. Co-authored-by: abi-jey <66185192+abi-jey@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

danielmeppiel · 2026-05-21T22:26:45Z

Follow-ups from the apm-review-panel pass have landed. Summary:

dst-symlink-write-anywhere: Add _assert_no_symlink_descendants(target) that walks each target_* destination with os.walk(followlinks=False) and raises PluginIntegrityError if the target itself or any descendant is a symlink. Wired into every copy site in _map_plugin_artifacts (agents, skills, prompts, hooks, and the .mcp.json / .lsp.json / settings.json pass-throughs). Refusing the copy is preferred over silent-skip because the threat model is data-loss-adjacent: a redirected copytree(..., dirs_exist_ok=True) write outside the plugin root is more dangerous than a failed install. Resolved in f8f7a9d.

Regression-trap evidence (mutation-break gate):

tests/unit/test_plugin_parser.py::TestMapPluginArtifactsPrePositioned::test_dst_symlink_in_target_does_not_redirect_copy -- deleted the _assert_no_symlink_descendants(target_agents) guard at src/apm_cli/deps/plugin_parser.py:500; test FAILED with DID NOT RAISE PluginIntegrityError; guard restored and test re-passes.

Lint contract: uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both silent (exit 0). Pylint R0801 duplication check also clean at 10.00/10.

CI: 12/12 required checks green on f8f7a9d -- Lint, APM Self-Check, Build & Test Shard 1/2 (Linux), Coverage Combine (Linux), PR Binary Smoke (Linux), Analyze (actions), Analyze (python), CodeQL, NOTICE Drift Check, gate, license/cla.

Ready for maintainer review.

abi-jey

@danielmeppiel I see that you already added assertion for sym link checks. lgtm!

abi-jey force-pushed the fix/plugin-artifacts-self-destruct branch from 01e1293 to a8845bf Compare May 20, 2026 09:31

abi-jey marked this pull request as ready for review May 20, 2026 09:32

abi-jey requested review from danielmeppiel and sergio-sisternes-epam as code owners May 20, 2026 09:32

Copilot AI review requested due to automatic review settings May 20, 2026 09:32

Copilot started reviewing on behalf of abi-jey May 20, 2026 09:34 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

Comment thread src/apm_cli/deps/plugin_parser.py Outdated

Comment thread tests/unit/test_plugin_parser.py

abi-jey marked this pull request as draft May 20, 2026 10:01

abi-jey force-pushed the fix/plugin-artifacts-self-destruct branch from a8845bf to 6cabc1f Compare May 20, 2026 11:45

abi-jey marked this pull request as ready for review May 20, 2026 11:48

abi-jey force-pushed the fix/plugin-artifacts-self-destruct branch from 6cabc1f to 0cf8057 Compare May 20, 2026 13:25

Merge branch 'main' into fix/plugin-artifacts-self-destruct

6561cfd

abi-jey commented May 21, 2026

View reviewed changes

danielmeppiel mentioned this pull request May 22, 2026

feat(batch-bug-shepherd): operator visibility + fold-in invariant #1451

Merged

danielmeppiel merged commit f37fe5a into microsoft:main May 22, 2026
12 checks passed

danielmeppiel mentioned this pull request May 23, 2026

[docs] Update documentation for features from 2026-05-22 #1458

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content#1416

fix(plugin): prevent _map_plugin_artifacts from destroying pre-positioned .apm/ content#1416
danielmeppiel merged 3 commits into
microsoft:mainfrom
abi-jey:fix/plugin-artifacts-self-destruct

abi-jey commented May 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

danielmeppiel commented May 21, 2026

Python Architect

CLI Logging Expert

DevX UX Expert

Supply Chain Security Expert

OSS Growth Hacker

Auth Expert -- inactive

Doc Writer

Test Coverage Expert

Performance Expert

Uh oh!

danielmeppiel commented May 21, 2026

Uh oh!

abi-jey left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

abi-jey commented May 20, 2026

Summary

Root Cause

Fix

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

danielmeppiel commented May 21, 2026

APM Review Panel: ship_with_followups

Panel summary

Top 5 follow-ups

Architecture

Recommendation

Python Architect

CLI Logging Expert

DevX UX Expert

Supply Chain Security Expert

OSS Growth Hacker

Auth Expert -- inactive

Doc Writer

Test Coverage Expert

Performance Expert

Uh oh!

danielmeppiel commented May 21, 2026

Uh oh!

abi-jey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

APM Review Panel: `ship_with_followups`

abi-jey left a comment •

edited

Loading